Repository navigation
fix(test): retain file ownership through isolation teardown - #162
Merged
Merged
Conversation
Wait for request and server cleanup, assert timeout notification and socket error ordering, and replace WAN abort races with held lookup and local pending-connection fixtures. Keep pending-abort parity with Node separate from Bun pre-aborted immediate-connect behavior.
Finalize file-owned handles and globals before releasing the active test owner and JUnit reporter. Mark teardown terminal without discarding callback metadata, and report uncaught cleanup errors through the existing file failure and worker IPC paths. Adapt the structured unhandled-error reporting pattern from oven-sh#44435 while preserving the existing cleanup callback policy. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
An isolated test file released its active-file and reporter scope before closing live handles and swapping its global. A user close callback could then throw without an owning test file: the run could remain successful and JUnit could omit the failure.
The canonical file runner now owns handle shutdown and the global swap before releasing the outgoing file's test/reporting scope.
begin_file_teardown()enters the terminal phase and cancels the active timer before callbacks run, so pending collection hooks cannot resume during finalization. Existing cleanup callbacks still run in their original order. Errors that reach the uncaught hook intentionally fail the run and are attributed once to the outgoing file through the existing serial/JUnit path or workerTestDonepayload. JUnit file finalization follows cleanup. The duplicated caller-level stop/swap paths are removed.The accompanying network tests repair the leaks and races that exposed this boundary:
ECONNRESETinstead of swallowing it.Raw production delta is +81 lines (+115/-34), or +82 excluding one trailing blank-line removal. This adds the file-lifetime ownership and structured error-reporting boundary. Tests are separate: +349 net lines (+448/-99); changelog +2.
The narrow synthetic file-error reporting pattern adapts oven-sh#44435. The native commit retains
Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>and the changelog credit.There is a policy overlap with oven-sh#44841: this fork intentionally tightens uncaught-error semantics during isolated cleanup while preserving callback delivery. An upstream submission needs explicit design review of that policy; this PR does not import either broader upstream change.
Deferred pre-existing follow-up:
RefDatadebug formatting can dereference a released collection scope. This is a debug-only source finding, with no runtime invalid-read reproduction. This PR does not change the logger; it remains a separate investigation.How did you verify your code works?
Exact signed native-proof head:
cd175f6191d04fa5438925d69a3be557b8743576, based onf3f6d6c33bd2a762e530cef167d04a5ff872cc96. Linux ARM64 release/LTO built throughbun bd --profile=release --lto=on --ci=on -j2, withCI,GITHUB_ACTIONS, andBUILDKITEunset so trailing test arguments execute. The fork's pinned published engine was used. NativeBun.revisionmatches that tested head, and all 20,075 tracked source files matched its tree during qualification.Current PR head
42c73e9090d6a0c6f511904d360c43df4c9f1909appends only a relocation of the existingSAFETYcomment immediately above its unsafe operand, fixing Clippy'sundocumented_unsafe_blocksfinding. The runtime code and tests are unchanged. Pinned Rust formatting passes; all nine named hosted checks pass on this head. The native results below belong tocd175f6191d04fa5438925d69a3be557b8743576and were not rerun for this comment-only change.Binary SHA256:
506da05105a1200439144e074adce63d65c021f0a0ac9a81a0b3e7302c0aeeab.test/cli/test/isolation.test.tstini 0.19.0 -s. The same zombie condition was reproduced on the preserved baseline; source and assertions were unchanged. Default child exit-code passthrough was verified.The original combined batch has zero GitHub
::errorannotations and zero unhandled-error reports. A handled TLSSocketProxyECONNRESETdiagnostic remains; this is not a claim that stderr contains no error-related text.Scoped canonical formatting and final
cargo fmt --all -- --check --verbosepass. Native ownership and fixture reviews are clear; scoped AutoReview found no actionable findings, and the final net runtime-selection change received separate review.The full frozen test-dependency installation did not complete. The selected dependency closure was verified, and every requested test above executed successfully. This is selected Linux ARM64 qualification, not a full cross-platform suite claim; hosted checks at 42c73e9 pass: Linux x64, macOS ARM64, Clippy, Miri, lol-html, mordant, JavaScript lint, source lints, and Format (https://github.com/openclaw/bun/actions/runs/38499206131).